Skip to content

[Server] Expire timed out client requests in Protocol::checkResponse() - #585

Open
Arslan-TR wants to merge 1 commit into
modelcontextprotocol:mainfrom
Arslan-TR:fix/561-timed-out-requests-cleanup
Open

Arslan-TR wants to merge 1 commit into
modelcontextprotocol:mainfrom
Arslan-TR:fix/561-timed-out-requests-cleanup

Conversation

@Arslan-TR

Copy link
Copy Markdown

Fixes #561

The unknown-ID half of the issue is already handled by handleResponse() on main; this covers the other half.

Problem

When a request to the client (elicitation, sampling) timed out, the polling loops of StreamableHttpTransport and StdioTransport resumed the fiber with a Request timed out error but never removed the entry from _mcp.pending_requests. Only Protocol::checkResponse() did that, and only for answers. So the entry stayed in the session until it ended, and a fiber that suspended again was resumed with the same stale timeout once more.

Change

  • Protocol::checkResponse() now returns the Request timed out error for a request past its timeout and drops its pending entry, like an answer does. An answer that is already stored still wins over the timeout.
  • The timeout branches (and their copies of the timestamp/timeout bookkeeping) are removed from both transports; checkForResponse() keeps its signature.
  • The expiry reloads the session before saving, so it does not write back a stale snapshot over what another stream stored in the meantime (ProtocolSessionRaceTest covers that case; its timed out request now returns the error instead of null). The general read-modify-write race stays with [Server][Streamable HTTP] Concurrent requests in the same session can overwrite queued responses and cause stale/unknown message IDs #275.
  • StreamableHttpTransport loses its $clock constructor argument, as the transport no longer compares timestamps itself. It was added after v0.8.1, so it is not in a release yet. Protocol uses time(), like handleResponse() already does.
  • CHANGELOG entry added.

Tests

New in ProtocolTest: the timed out request is reported once and only its entry is dropped, a request within its timeout stays pending, a stored answer beats the timeout, a late answer after the reported timeout is dropped, and a stream stops polling a request that timed out. The StreamableHttpTransport test that relied on the injected clock now checks that the polling loop resumes the fiber with the error the response finder returns.

ProtocolTest::testCheckResponseReportsTimedOutRequestAndDropsItsPendingEntry, testAnswerAfterReportedTimeoutIsDropped, testStreamStopsPollingTimedOutRequest and the updated ProtocolSessionRaceTest case fail without the src/ change.

Ran locally: phpunit unit suite, phpstan (no errors), php-cs-fixer on the changed files. The unit suite still has failures that are the same without this change (FileSessionStoreTest permission tests, JwtTokenValidatorTest, HttpTransportListenTest, on Windows). The integration tests that spawn a stdio server did not finish here, and DualEraEndpointTest::testAsksForInput fails identically on main; I did not run the interop/conformance suites.

The timeout branches in `StreamableHttpTransport` and `StdioTransport`
resumed the suspended fiber with an error but left the request in the
session's pending requests. A fiber that suspended again was then
resumed with that stale timeout once more, and the entry stayed in the
session until it ended.

`checkResponse()` now answers a request past its timeout with the
`Request timed out` error and drops its pending entry, like an answer
does, and the duplicated timeout logic is gone from both transports.
Expiry reloads the session before saving it back, so it does not undo
what other streams stored since the poll started.

`StreamableHttpTransport` loses its `$clock` constructor argument, which
only served the removed check.

Fixes modelcontextprotocol#561

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Server] Clean up timed-out client requests and drop responses nobody waits for

1 participant